AVRO-4296: [python] Bound zero-byte collection elements per datum, not per collection - #3926
Open
iemejia wants to merge 1 commit into
Open
AVRO-4296: [python] Bound zero-byte collection elements per datum, not per collection#3926iemejia wants to merge 1 commit into
iemejia wants to merge 1 commit into
Conversation
…t per collection The AVRO-4296 zero-byte-element cap (null, zero-length fixed, all-zero-byte records) was enforced per collection: read_array/read_map each started counting from zero. Because a container file carries its own schema, an attacker can declare a record with many array<null> fields, each block individually under the limit but jointly unbounded, so a tiny payload still drives a huge allocation (e.g. 16 array<null> fields of ~10M each: an ~80 byte record that exhausts memory, or 8 fields that burn tens of seconds of CPU). Track the cumulative zero-byte element count on the DatumReader across a single decoded datum, reset at the start of each top-level read() (the boundary DataFileReader uses per record), and check it in _ensure_collection_available. Positive-size elements are unchanged: they are naturally bounded per collection because decoding consumes input and the bytes-remaining check shrinks as the position advances. Adds regression tests for a multi-field record that exceeds the cap in aggregate and for a within-limit record that still reads (and confirms the budget resets between datums).
Member
Author
There was a problem hiding this comment.
Pull request overview
This PR strengthens the Python Avro DatumReader’s protection against amplification attacks by making the “zero-byte collection element” allocation cap cumulative across a single decoded datum (record), rather than resetting per collection field.
Changes:
- Track cumulative zero-byte collection elements read per top-level
DatumReader.read()viaself._zero_byte_items_read, and enforce the cap across the entire datum. - Update
_ensure_collection_availableto apply the zero-byte cap using the reader’s running total instead of a per-collection count. - Add unit tests covering multi-field record amplification and verifying the budget resets between separate datums.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lang/py/avro/io.py | Makes zero-byte element limiting cumulative per decoded datum by adding reader state and enforcing it in _ensure_collection_available. |
| lang/py/avro/test/test_io.py | Adds tests for multi-field record scenarios to ensure cumulative enforcement and reset-per-datum behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Follow-up to #3861 (AVRO-4296). That change capped the number of zero-byte-encoded collection elements (
null, a zero-lengthfixed, or a record whose fields are all zero-byte) that a decoder will allocate, since such elements consume no input and so cannot be bounded by the "bytes remaining" check.However, the cap was enforced per collection:
read_array/read_mapeach start counting from zero. Because an Avro container file carries its own schema, an attacker can declare a record with manyarray<null>fields, each block individually under the limit but jointly unbounded. A tiny payload therefore still drives a huge allocation:array<null>fields of ~10M each → an ~80-byte record that raisesMemoryErrorarray<null>fields → an ~40-byte record that burns tens of seconds of CPU while allocating ~80MNonereferencesThis PR makes the zero-byte-element budget cumulative across a single decoded datum rather than per collection:
DatumReadertracksself._zero_byte_items_read, reset at the start of each top-levelread()— the boundaryDataFileReaderuses per record._ensure_collection_availablechecks the running total for zero-byte elements.array<array<null>>are now also bounded in aggregate instead of getting a fresh budget per inner array.How was this patch tested?
test_record_of_array_of_null_fields_cumulative_across_datum(a multi-field record rejected once its combined zero-byte count exceeds the cap) andtest_record_of_array_of_null_fields_within_datum_limit_reads(a within-limit record still decodes, and the budget resets between datums).TestDatumReaderCollectionSizeLimitandtest_datafilesuites pass.array<null>still reads.An equivalent fix for the Java SDK is tracked separately.